Reset connection on return to pool #179
Conversation
The method completes synchronously, so the waiting isn't strictly necessary. However, the new code will rethrow any exception that the returned Task may have captured, which is useful for correctness (not that Dispose should throw an exception). Additionally, a call to `ConfigureAwait` without an `await` is confusing; this new code now uses the "standard" pattern for synchronously calling an async method.
|
This is an interesting proposition. I'll do some review and performance testing on it tomorrow. |
|
Use of Synchronous I/O in I think that awaiting the result of the Connection Reset should be deferred until the connection is drawn from the pool again. Since connections are inserted at the back of the Queue, this should give it a nice amount of time to complete before it is drawn again.
I pushed a branch with a commit on top of this PR that has this behavior: 5313c56 you should be able to cherry-pick that commit into this PR if it looks good. |
|
I think this will unintentionally fix #169 because the exception in |
|
As I wrote on #178:
I also don't currently have a good idea for handling this. Were you able to do any performance testing on this yet, to see if the change is worthwhile? |
If we want to mirror the current behavior, we could still initiate the reset task upon returning, but check for an exception upon re-drawing from the pool. It would essentially be removing this try-catch
The main benefit here would be for clients which exhibit behaviors: (A) Connection pool is not constantly in use Looking at our current traffic for drawing from the connection pool, we have 3 round trips:
RT 1 is from If the connection pool is constantly in use, there will be no difference whether connection is reset at the beginning or end because those events will occur back-to-back. If there is a negligible RTT (e.g. localhost like my timings here), then all of this will happen in a fraction of a millisecond. But, if (A) and (B) are both true, then we will move 2 of the 3 Round Trips when leasing a connection to a time when the connection would normally be idle, and that will benefit clients with a high RTT. One thing to consider is that by implementing this, we would now be forcing these 2 round trips over Sync I/O, which will block a thread pool thread. I don't think we force Sync I/O on any other happy paths in the codebase currently. Another option is to run |
|
As a follow-up to my most recent comment:
After reflecting on this a little more, I think that staying 100% async on the happy path is a primary goal of our library and we should not move forward with this change.
This is still an option but would mask errors in |
|
Closing in favour of #264. |
Issue #178 suggests that the connection be reset when it's returned to the pool, so that it's ready immediately when a new
MySqlConnectionis opened (and reuses a pooled connection).In the current PR, the connection is reset synchronously when the session is closed; this may not be sufficient to increase performance since the caller will still be blocked (albeit at the end of the connection lifetime, not at the beginning). It may be necessary to offload the work of resetting the connection to a background thread.